Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.63%. Comparing base (66054cc) to head (5c4f673).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main   #62912      +/-   ##
==========================================
- Coverage   91.48%   89.63%   -1.86%     
==========================================
  Files         358      708     +350     
  Lines      151591   220421   +68830     
  Branches    23925    42278   +18353     
==========================================
+ Hits       138690   197578   +58888     
- Misses      12625    14705    +2080     
- Partials      276     8138    +7862     

CI: https://ci.nodejs.org/job/node-test-pull-request/73051/

I noticed one extra behavior change from parsing the port with lastIndexOf(':') and parseInt(): it can pick up a colon that belongs to userinfo rather than the host port.

For example, current main keeps these at port 0 because there is no host port:

SocketAddress.parse('user:[email protected]')
SocketAddress.parse('user:[email protected]')

With this patch they become ports 80 and 81 respectively. That may be acceptable if userinfo-shaped inputs are considered out of scope, but it is a behavior change beyond preserving explicit default host ports.

An alternative is to keep using the HTTP URL parse for the address and existing non-default ports, and only when url.port is empty ask a scheme with no default port for the port value. For example:

URLParse(`socket://${input}`).port

That preserves the existing URL host parsing while recovering explicit host ports like :80.

I noticed one extra behavior change from parsing the port with lastIndexOf(':') and parseInt(): it can pick up a colon that belongs to userinfo rather than the host port.

For example, current main keeps these at port 0 because there is no host port:

SocketAddress.parse('user:[email protected]')
SocketAddress.parse('user:[email protected]')

With this patch they become ports 80 and 81 respectively. That may be acceptable if userinfo-shaped inputs are considered out of scope, but it is a behavior change beyond preserving explicit default host ports.

An alternative is to keep using the HTTP URL parse for the address and existing non-default ports, and only when url.port is empty ask a scheme with no default port for the port value. For example:

URLParse(`socket://${input}`).port

That preserves the existing URL host parsing while recovering explicit host ports like :80.

Great finding and solution, thank you @cookesan

I noticed one extra behavior change from parsing the port with lastIndexOf(':') and parseInt(): it can pick up a colon that belongs to userinfo rather than the host port.
For example, current main keeps these at port 0 because there is no host port:

SocketAddress.parse('user:[email protected]')
SocketAddress.parse('user:[email protected]')

With this patch they become ports 80 and 81 respectively. That may be acceptable if userinfo-shaped inputs are considered out of scope, but it is a behavior change beyond preserving explicit default host ports.
An alternative is to keep using the HTTP URL parse for the address and existing non-default ports, and only when url.port is empty ask a scheme with no default port for the port value. For example:

URLParse(`socket://${input}`).port

That preserves the existing URL host parsing while recovering explicit host ports like :80.

Great finding and solution, thank you @cookesan

Actually, I did some benchmarks and noticed the double URL.parse solution is definitely simpler but ~25% slower than a string-based approach. So I pushed the string solution.